Code Review — fix(federation): remove plaintext auth token from error log in getSharedSecret

Overview: getSharedSecret (@PublicPage) logged the valid stored token in plaintext on authentication failure: got "X" but expected "Y". Since any unauthenticated caller can trigger this log entry for any trusted server URL, anyone with log-read access could harvest the valid federation token and replay it. The fix removes the getToken() call and replaces the log message with a non-disclosing string.

Correctness

The change is minimal: one line removed (the $expectedToken DB fetch), one string literal updated. The isValidToken() check and the 403 response path are unchanged. ✅

Does dbHandler->getToken() get called twice now? No — isValidToken() internally calls getToken() and does the comparison itself. The additional getToken() call that was removed existed solely to populate the log message. Removing it is correct. ✅

Test

testGetSharedSecretDoesNotLogTokenValues asserts that the logger->error message contains neither the attacker-supplied token nor the stored token:

$this->logicalAnd(
    $this->logicalNot($this->stringContains($attackerToken)),
    $this->logicalNot($this->stringContains($storedToken))
)

This directly tests the security property, not just that a log call happened. The test would fail against the old code (which embeds both values) and pass against the new code. ✅

Note in PR body: "merge that one first to avoid conflicts" (referring to #41579). Both PRs touch OCSAuthAPIController.php but at different line ranges, so a clean merge order matters. Review team should coordinate.

Summary

Verdict: Ready to merge (after #41579).

Removes the submitted/expected federation shared-secret token values from the error log message in OCSAuthAPIController::getSharedSecret().

Findings

• Security — CWE-532 (Insertion of Sensitive Information into Log File), OWASP A09:2021: the fix for the stated call site is correct and well-tested (new test asserts neither token value appears in the logged message). However, the changelog's claim that "token values have been removed from the log message" is incomplete — apps/federation/lib/BackgroundJob/GetSharedSecret.php sends the same token as a GET query parameter, and on any non-403 HTTP failure calls $this->logger->logException($e, ...). Guzzle exception messages conventionally embed the full request URI including the query string, so the plaintext token can still leak into logs via this different, untouched code path. RequestSharedSecret.php already sends its token via POST body — the same pattern should be applied here, or the query string should be scrubbed before exception logging.
• Stability: no findings — isValidToken()'s actual auth decision is untouched, only the log line changed.
• Performance: net positive — removes a redundant DB lookup (getToken($url)) that existed only to populate the log string.
• Test coverage: good for the fixed call site; no coverage exists (or is feasible without deeper refactor) for the GetSharedSecret.php leak path noted above.

Verdict

Changes requested — the fix is correct as far as it goes, but ships an incomplete claim ("token removed from log") while a second, real leak path remains open in the same feature area. Recommend fixing GetSharedSecret.php in the same PR (or immediately after) rather than merging with the overclaim uncorrected.

🤖 Automated review by Claude Code (security · stability · performance · coverage)

Generated by Claude Code